Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #5357: Remove CDATA from translatable strings. #5361

Merged
merged 1 commit into from
Mar 15, 2024

Conversation

BenHenning
Copy link
Member

@BenHenning BenHenning commented Mar 14, 2024

Explanation

Fixes #5357

This PR removes all CDATA declarations in the translatable strings.xml file and instead escapes all necessary characters: < and > (& didn't need to be escaped since no strings use that character at the moment). This is needed because Translatewiki doesn't seem to extract the HTML within the CDATA declaration correctly, so it may not be translated (some existing strings were never translated, and per #5274 the latest FAQ changes aren't being processed correctly (leading to empty translation strings being submitted).

This PR also introduces a regex check + test to ensure that CDATA isn't used anymore in strings.

Essential Checklist

  • The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • Any changes to scripts/assets files have their rationale included in the PR explanation.
  • The PR follows the style guide.
  • The PR does not contain any unnecessary code changes from Android Studio (reference).
  • The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
  • The PR is assigned to the appropriate reviewers (reference).

For UI-specific PRs only

See video for a demonstration on all of the affected strings still rendering correctly (as far as I can tell) after this change:

demo_reduced.mp4

Also, add a regex check + test to ensure this doesn't regress in the
future.
@BenHenning BenHenning marked this pull request as ready for review March 15, 2024 00:18
@BenHenning BenHenning requested review from a team as code owners March 15, 2024 00:18
@BenHenning
Copy link
Member Author

@adhiamboperes PTAL.

@BenHenning BenHenning enabled auto-merge (squash) March 15, 2024 00:18
@BenHenning
Copy link
Member Author

(Preliminarily enabling auto-merge in case no issues are found since I might not be able to merge this before the next Translatewiki push).

Copy link
Collaborator

@adhiamboperes adhiamboperes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @BenHenning!

@BenHenning BenHenning merged commit 0cc0237 into develop Mar 15, 2024
38 checks passed
@BenHenning BenHenning deleted the fix-cdata-translations-issue branch March 15, 2024 01:09
adhiamboperes added a commit that referenced this pull request May 22, 2024
BenHenning added a commit that referenced this pull request May 26, 2024
Reverts #5361

There are some formatting issues that cause incorrectness in the HTML
structure, so @translatewiki cannot parse them correctly. This is the
cause of empty translatiion strings in #5274. Mainly, some elements are
missing their closing tags, e.g.:

```xml 
<string name="faq_answer_bug_reporting">&lt;p&gt;&lt;ol&gt;&lt;li&gt;From your %s app home screen, tap the menu in the top left corner.&lt;/li&gt;&lt;li&gt;Tap &lt;strong&gt;Share feedback&lt;/strong&gt;.&lt;/li&gt;&lt;li&gt;Follow the instructions to report the bug or share feedback.&lt;/li&gt;&lt;/p&gt;</string>
```
is missing an \</ol\> tag before the closing \</p\> tag.

I have looked at several of the other "broken" strings, and they have
similar issues. When @translatewiki encounters an unclosed tag, it
continues reading until it encounters a corresponding closing tag, in a
subsequent string, meaning any strings between the two points are
assumed to be part of a single string. This is obviously a serious bug
in translatewiki itself.

I think it is best to revert this PR and fix the issues first before
merging it again.

Co-authored-by: Ben Henning <[email protected]>
BenHenning pushed a commit that referenced this pull request Sep 10, 2024
<!-- READ ME FIRST: Please fill in the explanation section below and
check off every point from the Essential Checklist! -->
## Explanation

Fixes #5357

This PR includes the changes previously included in #5361, with an
additional fix for missing closing html tags described in #5403:

```
faq_answer_app_language: Missing </strong> tag after the email address.
faq_answer_bug_reporting: Missing closing </ol> tag.
faq_answer_exploration_player: Missing closing </ul> tag.
faq_answer_update_app: Missing closing </ol> tag and closing </p> tag.
faq_answer_update_os: Missing closing </ol> tag and closing </p> tag.
```

This PR removes all CDATA declarations in the translatable strings.xml
file and instead escapes all necessary characters: < and > (& didn't
need to be escaped since no strings use that character at the moment).
This is needed because Translatewiki doesn't seem to extract the HTML
within the CDATA declaration correctly, so it may not be translated
(some existing strings were never translated, and per
#5274 the latest FAQ changes
aren't being processed correctly (leading to empty translation strings
being submitted).

This PR also introduces a regex check + test to ensure that CDATA isn't
used anymore in strings.

## Essential Checklist
<!-- Please tick the relevant boxes by putting an "x" in them. -->
- [x] The PR title and explanation each start with "Fix #bugnum: " (If
this PR fixes part of an issue, prefix the title with "Fix part of
#bugnum: ...".)
- [x] Any changes to
[scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets)
files have their rationale included in the PR explanation.
- [x] The PR follows the [style
guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide).
- [x] The PR does not contain any unnecessary code changes from Android
Studio
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)).
- [x] The PR is made from a branch that's **not** called "develop" and
is up-to-date with "develop".
- [x] The PR is **assigned** to the appropriate reviewers
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)).

## For UI-specific PRs only
<!-- Delete these section if this PR does not include UI-related
changes. -->
If your PR includes UI-related changes, then:
- Add screenshots for portrait/landscape for both a tablet & phone of
the before & after UI changes
- For the screenshots above, include both English and pseudo-localized
(RTL) screenshots (see [RTL
guide](https://github.com/oppia/oppia-android/wiki/RTL-Guidelines))
- Add a video showing the full UX flow with a screen reader enabled (see
[accessibility
guide](https://github.com/oppia/oppia-android/wiki/Accessibility-A11y-Guide))
- For PRs introducing new UI elements or color changes, both light and
dark mode screenshots must be included
- Add a screenshot demonstrating that you ran affected Espresso tests
locally & that they're passing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG]: CDATA causes issues with Translatewiki importing
2 participants